Speed up Draco loading #6420

ggetz · 2018-04-03T19:54:29Z

Optionlly have TaskProcessor load binaries and compile Web Assembly modules in web workers
Fallback to a js module if web assembly is not supported
Update draco wasm module
Update draco test data

TODO:

Fix release test
Caching? (Probably open an issue for later)
Destroy attributeData
Swap out draco modules for gltf + point clouds

…aco test data

cesium-concierge · 2018-04-03T19:54:31Z

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

lilleyse · 2018-04-03T20:11:25Z

Cool! Do you have time comparisons between master and here?

ggetz · 2018-04-03T20:35:07Z

Making some now.

ggetz · 2018-04-03T21:01:26Z

For the NYC Dataset:

	`ScreenSpaceError`	Load Time
No Draco	4	86.7
No Draco	16	3.05
Draco - master	4	61.85
Draco - master	16	7.28
Draco - wasm	4	43.84
Draco - wasm	16	4.44

So faster overall, and much better once the module is compiled once and we're just loading a lot of tiles.

ggetz · 2018-04-03T21:09:37Z

Test are failing only in test release, I'll take a look at that.

pjcozzi · 2018-04-04T12:21:29Z

@ggetz why is Draco - wasm slower than No Draco for SSE = 16? Because not many tiles are loaded so the one time cost of compiling dominates (which I thought was cached by browsers btw)?

ggetz · 2018-04-04T13:49:33Z

why is Draco - wasm slower than No Draco for SSE = 16? Because not many tiles are loaded so the one time cost of compiling dominates (which I thought was cached by browsers btw)?

Correct. I've been testing the numbers with no caching, caching does not come for free, but is possible to implement (https://developer.mozilla.org/en-US/docs/WebAssembly/Caching_modules).

lilleyse

I'm definitely seeing a nice speedup here. Works well in Chrome, Firefox, and Edge.

Overall the code looks good but I think it would be best if someone with more experience in the TaskProcessor side of Cesium could take a look as well. Maybe @shunter?

lilleyse · 2018-04-04T17:05:05Z

CHANGES.md

+### 1.45 - 2018-05-01
+
+##### Additions :tada:
+* Added `webAssemblyOptions` parameter to `TaskProcessor` to specify options for loading a Web Assembly module in a web worker.


Add links to the PR in the new entries.

lilleyse · 2018-04-04T17:08:11Z

CHANGES.md

+### 1.45 - 2018-05-01
+
+##### Additions :tada:
+* Added `webAssemblyOptions` parameter to `TaskProcessor` to specify options for loading a Web Assembly module in a web worker.


Mention the addition to FeatureDetection.

lilleyse · 2018-04-04T17:20:04Z

Specs/Core/FeatureDetectionSpec.js

@@ -20,6 +20,11 @@ defineSuite([
        expect(typeof supportsTypedArrays).toEqual('boolean');
    });

+    it('detects typed array support', function() {


Change test name.

lilleyse · 2018-04-04T17:24:02Z

gulpfile.js

@@ -62,6 +62,7 @@ var sourceFiles = ['Source/**/*.js',
                   '!Source/Workers/**',
                   '!Source/ThirdParty/Workers/**',
                   '!Source/ThirdParty/draco-decoder-gltf.js',


draco-decoder-gltf got renamed.

lilleyse · 2018-04-04T17:25:25Z

CHANGES.md

+* Added `webAssemblyOptions` parameter to `TaskProcessor` to specify options for loading a Web Assembly module in a web worker.
+
+##### Fixes :wrench:
+* Faster loading of Draco compressed glTF assets.


Make some mention of WebAssembly, maybe "Faster loading of Draco compressed glTF assets in browsers that support WebAssembly".

lilleyse · 2018-04-04T17:41:36Z

Source/Workers/decodeDraco.js

-        draco,
-        createTaskProcessorWorker) {
+        createTaskProcessorWorker
+    ) {


Formatting: change back to createTaskProcessorWorker) {

lilleyse · 2018-04-04T17:44:30Z

Source/Workers/decodeDraco.js

@@ -125,6 +127,7 @@ define([

    function decodeDracoPrimitive(parameters) {
        if (!defined(dracoDecoder)) {
+            draco = self.wasmModule;


Is there a better way to get the wasm module? This doesn't feel right.

Or is this standard procedure for web worker tasks?

This isn't necessarily standard. @shunter Do you know if there was a more straightforward way of passing this module to the web worker functions without altering their function signatures (too much).

Well it doesn't seem like there's actually any relationship between loading Web Workers and loading a Web Assembly module, right? Why wouldn't the decodeDraco worker just load the module itself (using whatever helper method necessary)?

Right now it's assumed all web worker function are synchronous, and compiling the module is always asynchronous. I could restructure createTaskProcessorWorker to wait for a promise to be fulfilled if one is returned.

But additionally, which module to require is based on whether or not Web Assembly is supported, as we pass the backup js module if it is not. That's why I have this separate from the decodeDraco worker itself, as I don't want to require both modules.

I see now that you're actually loading the wasm binary on the main thread and transferring the data over to the worker.

I guess in general, my suggestion is that the protocol between the the main thread (DracoLoader) and the worker (decodeDraco) can be whatever you want.

Our existing workers have sort of a two-stage loading process:

The Worker is instantiated with cesiumWorkerBootstrapper, a special Worker which only understands bootstrap messages.

TaskProcessor posts a bootstrap message describing what worker module to transmogrify into. cesiumWorkerBootstrapper loads that module and replaces onmessage in order to hand off control.

At this point all of our existing workers are fully-formed (I think) but that doesn't necessarily have to be the case. You could add additional loading steps of your own design. decodeDraco could expect its first message to contain the wasm binary or the path to the fallback script, which it could then load synchronously (which is preferable in a worker). Then, it could switch modes and expect subsequent messages to be traditional task messages and process as usual.

On the main thread, DracoLoader could handle this protocol transparently by creating the TaskProcessor and then immediately scheduling the first message to finish the load.

Ultimately you can do what you want, but the point of all this would be to tease apart these two concepts rather than fusing them together such that they have to know about each other.

lilleyse · 2018-04-04T18:02:27Z

Source/Core/TaskProcessor.js

@@ -6,8 +6,10 @@ define([
        './destroyObject',


Even though Draco doesn't work in IE yet I went to open Sandcastle in IE and it doesn't get past the loading screen. master seems fine. Is this a result of any of the changes here?

Merged in master, that should incorporate the error message fix.

lilleyse · 2018-04-10T13:41:19Z

@ggetz with point cloud decoding coming soon I think it would make sense to replace the _gltf suffixed draco files with the non-suffixed versions here, and squash commits to save space.

Alternatively we could ship two versions, a _gltf version and a _pointcloud version. That would mean building the _pointcloud version ourselves (https://github.com/google/draco#webassembly-point-cloud-only-decoder). This would reduce upfront decode cost by cutting the wasm file by 100kb, but maybe not worth it.

ggetz · 2018-04-10T14:21:36Z

When we build the modules ourselves (CC #6404), it might be worth splitting them up since the initial decoding will be faster with smaller modules.

For now I think the best solution is to swap out for the non-gltf modules here. Added TODO to the PR description.

ggetz · 2018-04-11T21:38:48Z

@lilleyse This should be ready for anther look.

lilleyse · 2018-04-22T19:24:49Z

Fixes #6480

lilleyse · 2018-04-23T14:16:57Z

Source/Core/TaskProcessor.js

@@ -182,6 +212,7 @@ define([
        this._activeTasks = 0;
        this._deferreds = {};
        this._nextID = 0;
+        this._supportsWasm = FeatureDetection.supportsWebAssembly(); // exposed for testing purposes


Instead of adding this member variable for testing purposes, the test could mock FeatureDetection.supportsWebAssembly returning false.

lilleyse · 2018-04-23T15:36:31Z

Source/Scene/DracoLoader.js

    DracoLoader._getDecoderTaskProcessor = function () {
        if (!defined(DracoLoader._decoderTaskProcessor)) {
-            DracoLoader._decoderTaskProcessor = new TaskProcessor('decodeDraco', DracoLoader._maxDecodingConcurrency);
+            var processor = new TaskProcessor('decodeDraco', DracoLoader._maxDecodingConcurrency);
+            processor._supportsWasm = false;


This line should be removed.

lilleyse · 2018-04-23T15:39:08Z

Source/Core/TaskProcessor.js

+        // Web assembly not supported, use fallback js module if provided
+        if (!processor._supportsWasm) {
+            if (defined(wasmOptions.fallbackModulePath)) {
+                config.modulePath = buildModuleUrl(wasmOptions.fallbackModulePath);


What happens if wasm is not supported and fallbackModulePath is not provided? Should an error be thrown?

lilleyse · 2018-04-23T15:57:21Z

Source/Workers/decodeDraco.js

+
+        // Expect the first message to be to load a web assembly module
+        var wasmConfig = data.webAssemblyConfig;
+        if (wasmConfig !== undefined) {


Use defined here?

lilleyse · 2018-04-23T15:57:39Z

Source/Workers/decodeDraco.js

+            // Require and compile WebAssembly module, or use fallback if not supported
+            return require([wasmConfig.modulePath], function() {
+                var dracoModule = self.DracoDecoderModule;
+                if (wasmConfig.wasmBinaryFile !== undefined) {


lilleyse · 2018-04-23T16:09:09Z

CHANGES.md

@@ -3,18 +3,21 @@ Change Log

 ### 1.45 - 2018-05-01

+##### Additions :tada:
+* Improved `MapboxImageryProvider` performance by 300% via `tiles.mapbox.com` subdomain switching. [#6426](https://github.com/AnalyticalGraphicsInc/cesium/issues/6426)
+* Added `initWebAssemblyModule` function to `TaskProcessor` to loading a Web Assembly module in a web worker. [#6420](https://github.com/AnalyticalGraphicsInc/cesium/pull/6420)


Small typo: to loading

lilleyse · 2018-04-23T17:06:12Z

Draco 1.3.0 was released recently so we should update our Draco related files.

One nice change is:

Added support for all signed and unsigned integer types

So there are a lot more functions for retrieving attributes now: https://github.com/google/draco/blob/master/src/draco/javascript/emscripten/draco_web_decoder.idl#L221-L236

lilleyse · 2018-04-23T17:07:22Z

Offline we had talked about supporting more data_type variants besides 4 (UNSIGNED_SHORT). Here is the list I created when working on point clouds:

function getComponentDatatypeFromAttributeDataType(attributeDataType) {
    // Some attribute types are casted down to 32 bit since Draco only returns 32 bit values
    switch (attributeDataType) {
        case 1: return ComponentDatatype.BYTE;           // DT_INT8
        case 2: return ComponentDatatype.UNSIGNED_BYTE;  // DT_UINT8
        case 3: return ComponentDatatype.SHORT;          // DT_INT16
        case 4: return ComponentDatatype.UNSIGNED_SHORT; // DT_UINT16
        case 5: return ComponentDatatype.INT;            // DT_INT32
        case 6: return ComponentDatatype.UNSIGNED_INT;   // DT_UINT32
        case 7: return ComponentDatatype.INT;            // DT_INT64
        case 8: return ComponentDatatype.UNSIGNED_INT;   // DT_UINT64
        case 9: return ComponentDatatype.FLOAT;          // DT_FLOAT32
        case 10: return ComponentDatatype.FLOAT;         // DT_FLOAT64
        case 11: return ComponentDatatype.BYTE;          // DT_BOOL
    }
}

lilleyse · 2018-04-23T17:08:55Z

Given some of the architectural updates, it would be good to confirm that the performance numbers from #6420 (comment) are still correct.

ggetz · 2018-04-23T21:00:44Z

New numbers for the NYC Dataset:

	`ScreenSpaceError`	Load Time
No Draco	4	87.9
No Draco	16	2.99
Draco - master	4	61.5
Draco - master	16	7.32
Draco - wasm	4	43.4
Draco - wasm	16	4.34

ggetz · 2018-04-23T21:37:11Z

Thanks @lilleyse !

Addressed your comments, updated Draco, and updated numbers.

lilleyse · 2018-04-24T14:24:21Z

The new code changes look good.

I just want to be sure about the numbers - are all the tilesets in #6420 (comment) gzipped whereas none of the old tilesets in #6420 (comment) were gzipped?

Also do you know what might explain the much smaller gap between Draco - master and Draco - wasm for SSE = 4? Is it due to the 1.3.0 upgrade?

ggetz · 2018-04-24T14:47:56Z

I just want to be sure about the numbers - are all the tilesets in #6420 (comment) gzipped whereas none of the old tilesets in #6420 (comment) were gzipped?

Correct

Also do you know what might explain the much smaller gap between Draco - master and Draco - wasm for SSE = 4? Is it due to the 1.3.0 upgrade

I found the original tilesets I ran the numbers against, I'll use that and update the numbers in my second comment.

ggetz · 2018-04-24T16:01:37Z

@lilleyse Updated numbers, they are pretty much identical.

lilleyse · 2018-04-24T16:28:44Z

Thanks @ggetz!

ggetz added 2 commits April 3, 2018 15:35

Load web assembly in web workers, update draco wasm module, update dr…

0c9de68

…aco test data

Cleanup docs, update CHANGES.md

00bc404

ggetz requested a review from lilleyse April 3, 2018 19:54

lilleyse reviewed Apr 4, 2018

View reviewed changes

ggetz added 2 commits April 5, 2018 16:04

Cleanup, updaet module loading

10df9c7

Merge branch 'master' into wasm

4298c08

ggetz added 5 commits April 11, 2018 15:48

Restrcuture wasm loading, use full (not gltf-only) draco decoders

c0a4f86

Merge branch 'master' into wasm

c34ba36

Cleanup tests

1a07fe3

Destroy attribute data

c27c907

Fix loading in release

76b26a4

lilleyse mentioned this pull request Apr 22, 2018

Draco compressed glTF skinning model is not displayed #6480

Closed

lilleyse reviewed Apr 23, 2018

View reviewed changes

ggetz added 2 commits April 23, 2018 15:39

Update to Draco 1.3.0, cleanup

696e191

Merge branch 'master' into wasm

bb60b41

ggetz force-pushed the wasm branch from 2f8d084 to 49e8eda Compare April 23, 2018 19:49

Cleanup specs

4d2c551

ggetz force-pushed the wasm branch from 49e8eda to 4d2c551 Compare April 23, 2018 21:00

lilleyse merged commit 757d7b9 into master Apr 24, 2018

lilleyse deleted the wasm branch April 24, 2018 16:28

mramato mentioned this pull request Apr 26, 2018

Use legacy environment enabled build for draco decoder module #6404

Closed

Speed up Draco loading #6420

Speed up Draco loading #6420

Conversation

ggetz commented Apr 3, 2018 • edited

cesium-concierge commented Apr 3, 2018

lilleyse commented Apr 3, 2018

ggetz commented Apr 3, 2018

ggetz commented Apr 3, 2018 • edited

ggetz commented Apr 3, 2018

pjcozzi commented Apr 4, 2018

ggetz commented Apr 4, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Apr 10, 2018 • edited

ggetz commented Apr 10, 2018 • edited

ggetz commented Apr 11, 2018

lilleyse commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Apr 23, 2018 • edited

lilleyse commented Apr 23, 2018

lilleyse commented Apr 23, 2018

ggetz commented Apr 23, 2018 • edited

ggetz commented Apr 23, 2018

lilleyse commented Apr 24, 2018

ggetz commented Apr 24, 2018

ggetz commented Apr 24, 2018

lilleyse commented Apr 24, 2018

ggetz commented Apr 3, 2018 •

edited

ggetz commented Apr 3, 2018 •

edited

lilleyse commented Apr 10, 2018 •

edited

ggetz commented Apr 10, 2018 •

edited

lilleyse commented Apr 23, 2018 •

edited

ggetz commented Apr 23, 2018 •

edited